Code Review: MatrixDataSourceCodeFixProvider

Overall this is clean, well-scoped work that fits neatly into the existing code-fix pattern. The two-test approach (method + class) covers both diagnostic sites. A few observations:

Minor: SyntaxFactory.ParseName vs SyntaxFactory.IdentifierName

// Current
SyntaxFactory.ParseName("MatrixDataSource")

// More idiomatic for an unqualified name
SyntaxFactory.IdentifierName("MatrixDataSource")

ParseName triggers a full parse pass and is intended for potentially-qualified names (e.g., "TUnit.Core.MatrixDataSource"). For a simple identifier, IdentifierName is the canonical choice and is used throughout Roslyn's own code-fix providers. Not a correctness issue, but prefer the precise API.

Minor: Consistency — const string Title pattern

All existing fixers (e.g., VirtualHookOverrideCodeFixProvider) extract the title into a const so it's shared between title: and equivalenceKey::

private const string Title = "Add [MatrixDataSource]";

// then:
CodeAction.Create(title: Title, ..., equivalenceKey: Title)

The PR uses two separate string literals. If the title is ever renamed, it's easy to update one and forget the other (which breaks fix-all equivalence grouping). Extracting to a const keeps them in sync automatically.

Minor: Redundant using TUnit.Analyzers; in peer file

VirtualHookOverrideCodeFixProvider.cs explicitly imports using TUnit.Analyzers; for Rules, while this file omits it and relies on enclosing-namespace resolution (TUnit.Analyzers.CodeFixers is a child of TUnit.Analyzers). It compiles either way, but adding the explicit import would be consistent with the neighbouring fixer.

Not an issue: Missing using TUnit.Core;

The concern "what if the file doesn't have using TUnit.Core;?" doesn't apply here — the diagnostic only fires when [Matrix] is already present, and [Matrix] itself requires TUnit.Core, so the using is guaranteed to exist.

Not an issue: DocumentEditor vs document.WithSyntaxRoot

The choice of DocumentEditor (vs the raw WithSyntaxRoot approach in VirtualHookOverrideCodeFixProvider) is fine — DocumentEditor is actually the recommended pattern for additive changes and integrates better with the workspace's formatting pipeline.

Summary: The core logic is correct and the test coverage is solid. The three minor points above are all about consistency with existing conventions rather than correctness. Happy to approve once/if any of those are addressed, or as-is if the author prefers to leave them.

Follow-up Review

The previous feedback has been addressed:

• ✅ used (not )
• ✅ extracted and used for both and

The one remaining minor point from last round — adding an explicit using TUnit.Analyzers; for consistency with VirtualHookOverrideCodeFixProvider — is cosmetic and compiles fine either way. Not a blocker.

One new observation: fallback is unreachable

SyntaxNode updated = target switch
{
    MethodDeclarationSyntax method => method.AddAttributeLists(attributeList),
    TypeDeclarationSyntax type    => type.AddAttributeLists(attributeList),
    _ => target,   // dead code
};

The _ branch makes editor.ReplaceNode(target, target) a no-op. TUnit0049 can only be diagnosed on a method or a type, so this branch is never reached. Consider replacing it with _ => throw new InvalidOperationException($"Unexpected node kind: {target.Kind()}")` — this turns a silent no-op (wrong fix silently produces unchanged code) into a loud invariant violation that surfaces during development/testing if the analyzer ever fires somewhere unexpected.

Overall

The implementation is correct, well-tested, and consistent with the existing code-fix patterns. The two-test suite covers both diagnostic sites cleanly. Ready to merge.

Follow-up Review

The previous feedback has been addressed:

• SyntaxFactory.IdentifierName used (not ParseName)
• private const string Title extracted and used for both title: and equivalenceKey:

The one remaining minor point from last round (explicit using TUnit.Analyzers; for consistency) is cosmetic and compiles fine. Not a blocker.

New observation: _ => target fallback is a silent no-op

The _ arm in the switch produces editor.ReplaceNode(target, target) — replacing a node with itself. Since TUnit0049 only fires on methods and types, this branch is unreachable. The concern is that a silent no-op is worse than a loud failure: if the analyzer ever fires somewhere unexpected, the fix would appear to succeed but produce no visible change.

Consider replacing it with an exception:

_ => throw new InvalidOperationException($"Unexpected node kind: {target.Kind()}")

This turns a hidden invariant violation into an immediate, diagnosable error during development or testing, rather than a silently-wrong fix.

Overall

The implementation is correct and the two tests cover both diagnostic sites cleanly. Previous review feedback has been fully addressed. Ready to merge once the unreachable branch is handled (or intentionally kept as a safe fallback — either is defensible).

Follow-up Review — All Feedback Addressed

All three points from previous rounds have been resolved in the current diff:

• ✅ SyntaxFactory.IdentifierName used (not ParseName)
• ✅ private const string Title extracted and shared between title: and equivalenceKey:
• ✅ _ => throw new InvalidOperationException(...) replaces the silent no-op fallback

The cosmetic point (explicit using TUnit.Analyzers;) is the only thing untouched, and it's not a blocker — the code compiles and behaves correctly either way.

Overall

The implementation is clean, correct, and consistent with the existing code-fix patterns. Both diagnostic sites (method and class) are covered by tests. Ready to merge.